Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raise all SystemCallErrors as Trilogy::Error #63

Merged

Conversation

adrianna-chang-shopify
Copy link
Collaborator

Background

See #58 for more context.

This allows syscall errors to be rescued generically by Trilogy::Error, or handled individually by consumers using their original Errno classes.

To make this work, we subclass all of the SystemCallErrors and include the base error module on each of them.

I've also removed the constructors from the existing Errno class subclasses (Trilogy::TimeoutError, Trilogy::ConnectionClosedError, etc.). We never set an error_code on syscall errors, so the constructors shouldn't be needed.

If anyone has input on a better way to handle including the Trilogy::Error module on all of the Errno classes, let me know! Enumerating descendants of SystemCallError, subclassing them, and including Trilogy::Error seems like the most straightforward approach so far, but is admittedly not pretty.

@adrianna-chang-shopify adrianna-chang-shopify force-pushed the ac-syscall-errs-inherit-from-trilogy-error branch 5 times, most recently from bc6c5a4 to b243b50 Compare March 27, 2023 13:41
@casperisfine
Copy link
Contributor

Enumerating descendants of SystemCallError, subclassing them, and including Trilogy::Error seems like the most straightforward approach so far, but is admittedly not pretty.

So:

Errno.constants.map { |c| Errno.const_get(c) }.select { |c| c.is_a?(Class) && c < SystemCallError }.

Is the proper way to do it I think. The only issue is:

>> Errno.constants.map { |c| Errno.const_get(c) }.select { |c| c.is_a?(Class) && c < SystemCallError }.size
=> 158

And the vast majority of them will never happen.

Alternatively, we can do it manually for a subset of ernno we care about, and have a fallback Trilogy::UnexpectedSyscallError for the ones we don't expect to happen aside from weird bugs.

@adrianna-chang-shopify adrianna-chang-shopify force-pushed the ac-syscall-errs-inherit-from-trilogy-error branch from b243b50 to e5a53a5 Compare March 27, 2023 15:55
@adrianna-chang-shopify
Copy link
Collaborator Author

Alternatively, we can do it manually for a subset of ernno we care about, and have a fallback Trilogy::UnexpectedSyscallError for the ones we don't expect to happen aside from weird bugs.

I'm not super confident identifying which errnos seem reasonable to care about, so as long as we're sticking to the module-based approach my vote would be to enumerate all of them. If there are more that we truly expect, maybe we should promote them to proper classes?

cc @matthewd any opinions here?

@matthewd
Copy link
Contributor

Yeah my concern is that it's really hard to define a list of plausibly-expectable errors (e.g. EHOSTUNREACH probably wouldn't make an initial cut, but considered properly does seem like a thing that an interested caller could reasonably want to specifically handle as a connection issue -> retry / whatever).

Not to mention the theoretical variation across platforms once we stray from the basics.

My first thought was to use a lazy hash... which doesn't feel like a particularly good idea, and would be messy though maybe not ridiculous. It's a bounded list, and most processes most of the time won't ever see / need most classes.

This feels terrible but I can't see an alternative that doesn't pre-encode a subset of errnos that are "allowed" to happen. 😕

contrib/ruby/lib/trilogy.rb Outdated Show resolved Hide resolved
contrib/ruby/lib/trilogy.rb Outdated Show resolved Hide resolved
@adrianna-chang-shopify
Copy link
Collaborator Author

adrianna-chang-shopify commented Mar 27, 2023

Oh actually lazy hash sounds like a good idea? e.g. something like this right?

  class SyscallError
    ERRORS = {}
    class << self
      def from_errno(errno, message)
        ERRORS.fetch(errno) do |errno|
          errno_name = Errno.constants.find { |c| Errno.const_get(c)::Errno == errno }
          errno_subclass = Class.new(Errno.const_get(errno_name)) { include Trilogy::Error }
          ERRORS[errno] = const_set(errno_name, errno_subclass)
        end.new(message)
      end
    end
  end

That way we avoid subclassing the unlikely ones. A bit messy, but not any worse than iterating all of em IMO 😅

@casperisfine
Copy link
Contributor

I'd advise against dynamic const_set. Until very recently (3.2) that bumps the global constant cache, which isn't good.

It also make it weird because you can't reference the error before it's actually raised.

That said, defining the 150-ish classes would use about 30kB, it's a indeed a bit much... but acceptable?

>> ObjectSpace.memsize_of(Class.new(StandardError)) * 154
=> 29568

@adrianna-chang-shopify
Copy link
Collaborator Author

Until very recently (3.2) that bumps the global constant cache

Oh good to know, thank you Jean 🙇‍♀️

That said, defining the 150-ish classes would use about 30kB

I think it would be closer to 20kbB; several of the Errno constants map to Errno::NOERROR. With #uniq:

>> Errno.constants.map { |c| Errno.const_get(c) }.uniq.select { |c| c.is_a?(Class) && c < SystemCallError }.size
=> 107
>> ObjectSpace.memsize_of(Class.new(StandardError)) * 107
=> 20544

@adrianna-chang-shopify
Copy link
Collaborator Author

@byroot @matthewd do we feel comfortable shipping this in its current form?

@byroot
Copy link
Contributor

byroot commented Apr 11, 2023

Fine by me.

@matthewd
Copy link
Contributor

I think @casperisfine was advising against the combo of const_set + deferred subclassing, not necessarily const_set + immediate-subclass loop.

Failing that, I think we'd want to define both self.inspect and inspect:

Class.new(Errno::EBADF).new
=> #<#<Class:0x0000000104db7e50>: Bad file descriptor>

@byroot
Copy link
Contributor

byroot commented Apr 13, 2023

not necessarily const_set + immediate-subclass loop.

Yeah, if it's eagerly defined it's fine.

@adrianna-chang-shopify
Copy link
Collaborator Author

Ahh gotcha, yeah that makes more sense in terms of invalidating the global constant cache. Let me bring back back the const_set then 👍

@adrianna-chang-shopify adrianna-chang-shopify force-pushed the ac-syscall-errs-inherit-from-trilogy-error branch from 8dc87ad to 4623eb5 Compare April 13, 2023 14:52
This allows syscall errors to be rescued generically by Trilogy::Error,
or handled individually by consumers using their oriinal Errno classes.

To make this work, we subclass all of the SystemCallErrors and include
the base error module on each of them.

I've also removed the constructors from the existing Errno class subclasses
(Trilogy::TimeoutError, Trilogy::ConnectionClosedError, etc.). We never
set an error_code on syscall errors, so the constructors are not needed.
@adrianna-chang-shopify adrianna-chang-shopify force-pushed the ac-syscall-errs-inherit-from-trilogy-error branch from 4623eb5 to c104e5c Compare April 13, 2023 14:53
Copy link
Contributor

@matthewd matthewd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@adrianna-chang-shopify adrianna-chang-shopify merged commit ca99504 into main Apr 14, 2023
@adrianna-chang-shopify adrianna-chang-shopify deleted the ac-syscall-errs-inherit-from-trilogy-error branch April 14, 2023 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants